Feat/batching circuit gadgets #396
Conversation
| row_path: MerklePathWithNeighborsGadget<ROW_TREE_MAX_DEPTH>, | ||
| index_path: MerklePathWithNeighborsGadget<INDEX_TREE_MAX_DEPTH>, | ||
| row_cells: &RowCells, | ||
| is_non_dummy_row: bool, |
There was a problem hiding this comment.
this is confusing in the double negation way, can we use the flag as dummy_row: bool or even better, an enum ?
There was a problem hiding this comment.
It's simpler to have this doubly-negated flag in circuit, but I can have the more intuitive version in the API and negate it in the assign function.
There was a problem hiding this comment.
Done in commit 5787dc7
| min_query_bound: &UInt256Target, | ||
| max_query_bound: &UInt256Target, | ||
| are_rows_tree_nodes: bool, | ||
| ) -> (BoolTarget, BoolTarget) { |
There was a problem hiding this comment.
Small comments to say what are the returned bools ?
There was a problem hiding this comment.
Done in commit 5787dc7
| // if second_node_pred_in_range is true, and the predecessor of the second node was found in the path from | ||
| // such node to the root of the tree, then the hash of predecessor node will be placed in | ||
| // `second..predecessor_info.hash` by `MerklePathWithNeighborsGadget: therefore, we can check that `second` | ||
| // is consecutive of `first` by checking that `second.predecessor_info.hash` is the hash of the first node; | ||
| // otherwise, we cannot check right now that the 2 nodes are consecutive, and it necessarily means we have | ||
| // already done it before when checking that the successor of first node was the second node |
There was a problem hiding this comment.
Can you clarify a bit this for me please ?
- if the successor of first node is not found, we'll check it later (from comments)
- if predecessor of second node is not found, that means we checked it already in the first point ?
do you mean either of these two conditions MUST be true at all times ? Can you tell me why quickly ?
There was a problem hiding this comment.
When both first and second are in the same tree, one of the conditions must be always true, I argued about this here.
Indeed, later in this method I compute also a flag either_is_in_path, which is true iff one of the 2 conditions is true, and I enforce this flag to be true if the 2 nodes are in the same tree.
Instead, both the conditions might be false if first and second doesn't belong to the same tree: for instance, if first is the last node in a rows tree, and second is the first node in another rows tree, then first has no successor, and second has no predecessor, so both the conditions will be false. However, in this case we don't need to bind successor of first with second, and vice versa, since the node belongs to different rows trees (we are doing other checks to enforce that the nodes are consecutive across rows trees), so it's not an issue to skip both these checks on successor/predecessor hash. Does it clarify the logic here?
| second_node_pred_in_range.target, | ||
| ); | ||
| let range_flags_xor = b.arithmetic( | ||
| F::NEG_ONE + F::NEG_ONE, |
There was a problem hiding this comment.
worth putting it in a constant ?
There was a problem hiding this comment.
Done in commit 5787dc7
| min_primary: &UInt256Target, | ||
| max_primary: &UInt256Target, | ||
| min_secondary: &UInt256Target, | ||
| max_secondary: &UInt256Target, |
There was a problem hiding this comment.
can you precise whether those are from the query or just from a node ? I assume the query but would be nice to have clear context all the time when talking about min and max.
There was a problem hiding this comment.
Renamed in commit 5787dc7
| /// This module contains gadgets to enforce whether 2 rows are consecutive | ||
| pub(crate) mod consecutive_rows; | ||
|
|
||
| /// Data structure containing the wires representing the data realted to the node of |
There was a problem hiding this comment.
Fixed in commit 5787dc7
| } | ||
|
|
||
| /// Data structure containing the `BoundaryRowNodeInfoTarget` wires for the nodes | ||
| /// realted to a given boundary row. In particular, it contains the |
There was a problem hiding this comment.
Fixed in commit 5787dc7
…ing_circuit_gadgets
…ing_circuit_gadgets
This PR adds the circuits to prove queries in a batching fashion, building up on the gadget introduced with previous PRs. These circuits should allow to scale better when proving queries. In terms of public parameters, these circuits will be currently available only by enabling the feature `batching_circuits`; this is a temporary solution to avoid including 2 different sets of circuits for the same task, and we will remove this feature once we will assess whether it's worthy to replace the existing query circuits with the batching ones. --------- Co-authored-by: T <[email protected]>
nikkolasg
merged commit fcf3030
into
feat/merkle-path-with-successor-gadget
This PR provides the gadgets necessary to build the batching query circuits, in particular: